Dan P <dan### [at] yahoocom> wrote:
> How can I make my code better?

  That depends on whether you definitely want to stick to C or if you would
like to do it the right way in C++... :)
  With C++ many things can be done a lot more nicely and safely (eg. minimize
the risk of memory leaks).
  However, let's make this the C-way for once. C is quite cumbersome in many
ways, but it may be sometimes good to know how things can be done better
there (you never know when you will be forced to use C in some project).
C offers some tools for better modularity, even though not many.

  So let's see your code:

> unsigned *buffer = NULL;
> char *page = NULL;
> unsigned units = 0;
> int frames = 0;
> int width;
> int height;
> unsigned x;
> unsigned y;
> unsigned x2;
> unsigned y2;
> unsigned w;
> unsigned h;

  I haven't looked yet if all those variables are needed, but anyways,
you should encapsulate them inside a struct. Besides not contamining
the global namespace it makes it easier to enhance your program in
the future. For example, if you want for some reason to keep many images
on memory at the same time it will be much easier having them in struct
instances.
  So perhaps something like this:

struct BlurData
{
    unsigned* buffer;
    char* page;
    unsigned units;
    int frames, width, height;
    unsigned x, y, x2, y2, w, h;
};

  Then in the main() function you can create an instance of this struct
which can be used in the functions it calls:

> int main (int argc, char **argv)
> {
      BlurData data = { NULL, NULL, 0, 0 };

>  int i = 1;
>
>  x = atoi(argv[i++]);
>  y = atoi(argv[i++]);
>  x2 = atoi(argv[i++]);
>  y2 = atoi(argv[i++]);

  Here you don't check if the number of command-line parameters is correct.
That is what 'argc' (argument count) is for.
  If the user didn't give the correct amount of parameters the program
should print a syntax reminder. Instead, your program misbehaves and
prints nothing helpful. It will probably cause a segmentation fault.

  Also, you don't check that the user gave correct parameters, and even
if the parameters are valid, if they make sense.
  atoi() will return 0 if the parameter is not a number. If you want to
go the easy way you could just define that if the user gives invalid
parameters it's the same as he had given a 0. However, you should at
least check that not all the four parameters are 0.

  Using 'i' to index fixed positions in 'argv' seems useless, but that
isn't so bad. It makes it easier to change the number of command-line
parameters.

  So you could perhaps do something like this:

if(argc < 6) /* we need at least 4 coordinates and one file */
{
    fprintf(stderr, "Syntax: %s <x1> <y1> <x2> <y2> <ppm files>\n", argv[0]);
    return EXIT_FAILURE;
}

data.x = atoi(argv[i++]);
data.y = atoi(argv[i++]);
data.x2 = atoi(argv[i++]);
data.y2 = atoi(argv[i++]);

if((data.x==0 && data.y==0 && data.x2==0 && data.y2==0) ||
   data.x2 < data.x || data.y2 < data.y)
{
    fprintf(stderr, "Invalid coordinate parameters.\n");
    return EXIT_FAILURE;
}

data.w = data.x2 - data.x;
data.h = data.y2 - data.y;

  The two last checks in the previous 'if' were so that w and h wouldn't get
screwed up.

>  while (argv[i] != NULL)

  You should use 'argc' to see where do the parameters end. I don't think
you can do that according to the C standard. 'argv[argc]' and beyond can
probably be anything, giving you trash (and if you are lucky causing a
segmentation fault).
  So it should be: while(i < argc)
  We can also do it this way:

for(i = 5; i < argc; ++i)
{
    load(argv[i], &data);
    ++data.frames;
}

  I think that it would be better if load() increased 'data.frames', but
that's a minor detail.

  Naturally blur() has to get the data as parameter now, so the call
would become: blur(&data);

>  free((void *) buffer);
>  free((void *) page);

  Why are you casting the the pointers to void* before freeing them?

  And by the way, this is one of the bad things about C. In C++ you could
make the struct to automatically free those arrays when it goes out of
scope.
  Doing it in the C-way (which you are pretty much forced to do, usually)
not only breaks modularity badly, but is also dangerous because it
increases the danger of memory leaks.

  If we were using C++, we could enhance our struct (which in fact should
be a class with the variables in its private part, but that's another
story) with a destructor, like this:

struct BlurData
{
    (The data here)

    ~BlurData
    {
        if(buffer) free(buffer);
        if(page) free(page);
    }
};

  Then the arrays will be automatically freed when 'data' goes out of
scope (so it doesn't matter where the program is terminated).

  Naturally if we were using C++ we wouldn't make it a struct at all,
but a fully-functional class which in itself makes the blur and handles
practically everything. (main() would simply create an instance of this
class and give it the file names.)

>  return 0;

  Usually you should return EXIT_SUCCESS from main() if you want to be
sure of portability.
  EXIT_SUCCESS is *usually* 0, but the standard does not specify that.

> void blur()
> {
>  unsigned i;
>  unsigned xn;
>  unsigned yn;
>  unsigned r;
>  unsigned g;
>  unsigned b;

  You can write that as:

unsigned i, xn, yn, r, g, b;

>  fflush(0);

  It's 'fflush(stdout);', and it's unneeded anyways.

> void load (char *path)

  It's a good practice to take that kind of parameter as "const char* path".
It not only avoids you accidentally modifying it, but it also avoids all
kinds of problems, specially in C++.

>  char s_version[1024];
>  char s_width[1024];
>  char s_height[1024];
>  char s_depth[1024];

  This should be something which immediately turns on every alarm in your
head.
  Static buffer overflow is the oldest and most common exploit of all time,
and it still is nowadays.

  Why? Because of the attitude of programmers: "This is a small program
it doesn't really matter". They don't get accustomed to writing secure
code, they don't know how to do it, and then when they write code to
a program which will become something important...

  Believe or not, people are still making this mistake, in the year 2004,
in crucially important programs which can be used to exploit systems.
It all starts with all those "small and unimportant" programs...

  Please don't teach yourself bad habits. Get rid of them from the very
start.

  Besides, you are reserving 4 kilobytes of memory for something which
probably needs a few bytes. It doesn't matter here, but it can start
mattering when the same thing is done millions of times...

  In C++ there's a safe way of parsing words from a stream by using
the std::getline() function. In C it requires more work to get a secure
dynamic string working.
  However, you don't even need dynamic strings in this case. For example,
you are storing some version information in 's_version' but you don't use
it anywhere. Also, you are storing numbers in char arrays for no apparent
reason.

  In C there's a great function for parsing files with a fixed content:
fscanf(). Learn to use it. You don't need temporary buffers for reading
integers with it.
  As for the version, you can simply "read it away" from the input file.
You don't need to store it anywhere.

>  if ((stream = fopen(path, "rb")) != NULL)
>  {

  In my personal opinion it's better to handle the failure first and
the success then. Now the failure handling is many tens of lines below,
and it's hard to find.
  As for the failure printing itself, it's a good habit to print the *reason*
why opening failed, not just that it failed.
  There's a nice function in C for doing exactly that: perror().

  And I have never understood why people use obfuscated C for no apparent
reason to open a file. Why it can't be opened and checked in two separate
lines? It's not like it would be less efficient or anything, but certainly
easier to read.

  So my suggestion is:

stream = fopen(path, "rb");
if(stream == NULL)
{
    fprintf(stderr, "Couldn't open ");
    perror(path);
    return;
}

  The fprintf/perror combination is a nice trick I figured out years ago
to print out a nicely-formatted error message.

>    buffer = (unsigned *) malloc(units * sizeof(int));

  Allocating space for ints but converting it to an unsigned array is
a bit odd. It usually works, but...
  Better safe than sorry, just use sizeof(unsigned). It's not that hard.

  By the way, you don't check whether the subsequent images have a proper
size with regard to the first image. It's a good idea to check that.
(For instance, if the subsequent images are bigger than the first one
you will be writing outside your array, causing a segmentation fault.
If they are smaller, the result will be quite odd.)

-- 
#macro M(A,N,D,L)plane{-z,-9pigment{mandel L*9translate N color_map{[0rgb x]
[1rgb 9]}scale<D,D*3D>*1e3}rotate y*A*8}#end M(-3<1.206434.28623>70,7)M(
-1<.7438.1795>1,20)M(1<.77595.13699>30,20)M(3<.75923.07145>80,99)// - Warp -

Post a reply to this message